-
Notifications
You must be signed in to change notification settings - Fork 781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[entropy_src] Fix handling of backpressure in the hardware pipeline #21846
Conversation
This commit cleans up and documents the control signals for the main FIFOs of the pipeline including the esrng, esbit, postht, precon and esfinal FIFOs. Most of the changes simplify the code but don't alter the behavior of the design as the used FIFO primitives already implement the logic to not accept pushes when full internally. However, there are some important changes that are necessary: 1. The esrng FIFO handles no backpressure anymore. This wasn't spec compliant. 2. The sample drop point in case of backpressure is moved to after the health tests in case we are not in firmware overide insert mode and the window counter controls are updated. This means in case of backpressure, samples are tested but they're not pushed into the postht FIFO (or the esbit FIFO in case of single-bit mode). The window timer doesn't increment to keep the number of samples ending up in the conditioner fixed, independent of backpressure. This is required by the spec. Signed-off-by: Pirmin Vogel <[email protected]>
This commit switches all instances of prim_fifo_sync to use hardened counters for the pointers. Signed-off-by: Pirmin Vogel <[email protected]>
This commit adds a 32-bit wide distribution FIFO of configurable depth. The FIFO is added between the postht FIFO, the observe FIFO, the bypass FIFO and the precon FIFO. Its main purpose is to buffer entropy bits while the conditioner is busy such that we don't have to drop entropy bits from the hardware pipeline. Dropping entropy bits is not a big issue per se during normal operation as it's allowed by the spec (when done after the health tests and in a way such that number of samples going into the conditioner is fixed). Also, under normal operating conditions, noise source samples arrive at very low rate and dropping bits should not be needed. However, verifying that the `correct` entropy bits are dropped is hard and seems impossible for our current DV environment as it requires to very accurately model the hardware pipeline which is undesirable. Thus, the safest approach is to add this new distribution FIFO and tune its depth parameter to handle potential backpressure from the conditioner such that dropping bits is not necessary. Signed-off-by: Pirmin Vogel <[email protected]>
This is useful to reduce the backpressure in the pipeline as this leads to entropy bits being dropped eventually which the scoreboard cannot handle at the moment. Signed-off-by: Pirmin Vogel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @h-filali. I checked the diff to #21799 and I think all our comments are addressed. From my side we can merge this (or let @vogelpi) pick your fixes to his branch and merge PR #21799. We should then follow up with issues and separate PRs on the open points in #21799, which are less urgent and shouldn't block this PR IMO.
Thanks @h-filali for your work on this. I've taken most of your changes and re-included them into the original PR. |
Closing as this has been integrated into #21799. |
Copied from #21799
Now that the the noise source is no longer disabled upon the esrng FIFO filling up, the hardware pipeline is no longer handling backpressure from the conditioner appropriately. This becomes very obvious with #21787 which fixes the max rate test as well as the CS AES halt interface (a main source of backpressure in the entropy source).
This PR contains a couple of commits to counteract this:
This is resolves #21686 and is related to #20953.
What's still required and not yet part of this PR:
Me chipping in with some comments
For the first of the three opens above I would argue that it's fine dropping in front of the bypass FIFO, since if the configuration is not FIPS compliant we don't really care about dropping data (as long as it's not dropped unnecessarily).
If the bypass configuration should be FIPS compliant, then dropping at the bypass FIFO is effectively the same as dropping at the esfinal FIFO.
IMO this is a question of efficiency, so ideally drops should only happen when the esfinal FIFO is full.
With regards to the firmware override mode I would like to add that I think we need to be 100 percent sure that we are not dropping any entropy before the observe FIFO. Here is my reasoning for this.
This could be done like @andreaskurth said:
"In a top-level test that runs validation/min entropy estimation on Ibex, check that no entropy is dropped within a 4 kibit window. (This test can be added in a later, DV-focused milestone.)"
We should make sure that that none of the FIFOs before the observe FIFO drop entropy in fw_ov_mode.
Another thing to consider would be this. To model the back pressure closer to reality we might want to consider reducing the max rate from the noise source instead of reducing the number of cycles until AES from CSRNG is done and the SHA can start it's operation.
@vogelpi I think we can merge this PR (which I stole from you) now and create some issues/follow up PRs at some point in the future, what do you think?